fix(#11324): recognise closing keywords on non-default-branch PRs by ken-talltree-io · Pull Request #11332 · medic/cht-core

ken-talltree-io · 2026-08-07T15:54:47Z

Description

The linked-issue check reads GitHub's GraphQL closingIssuesReferences, which GitHub only populates for PRs targeting the repository's default branch. On any other base the field is empty even when the contributor linked the issue correctly — so AndraBot failed the PR and asked them to add a closing keyword they had already added, with no way to clear it. That's what happened to @megha1807 on #11320.

When GitHub reports no linkage, getLinkedIssues now falls back to parsing the PR body for closing keywords and resolving each reference through the issues API. The results are shaped like closingIssuesReferences nodes, so the existing same-org filter and the assignee check work on them unchanged.

Recognised: #123, owner/repo#123, and full issue URLs, with GitHub's documented keyword set (close/closes/closed, fix/fixes/fixed, resolve/resolves/resolved).

Closes #11324

Since your review, @sugat009

All nine findings are fixed. Your two blocking ones were exactly right and I'd argued against half of one in the original body — the note there that unstripped code fences were "largely self-limiting" was wrong, because I'd missed that .some() made the assignment check pass on any mentioned issue.

Three things you should know before re-reading, because the diff has moved a long way:

I reverted one of the fixes you'd effectively asked for. Stripping code led to a first attempt that ran an unclosed fence to the end of the body, the way a renderer does. Three subsequent review passes showed that cure was worse than the disease: it produced three separate ways to discard a genuine issue link, each failing a contributor with a message telling them to add the link they had already added. Only a fence that actually closes now delimits a block, so an unpaired marker strips nothing.

That is a deliberate trade in one direction. Reading code as prose lets a PR that merely documents a link past a check that still requires the author be assigned to that issue. Reading prose as code blocks a real contributor with no way to clear it. The second is much worse, so every ambiguity now resolves toward stripping less. If you disagree with that ordering, this is the decision to push back on.

I introduced two performance defects while fixing performance defects, and you should weigh that. Fixing your .. finding by requiring a non-dot character ([\w.-]*[\w-][\w.-]*) made the quantifiers ambiguous and the failed match cubic — 68s on an 8k body, hours at the 65,536-character limit, on a pull_request_target body re-parsed on every edit. That was strictly worse than the ReDoS your own finding #3 was about. .. is now rejected after parsing instead, and the pattern is linear again.

Fixing that left the fence regex quadratic — 3.5s on a body of backticks at the same limit, where the previous implementation was linear. The opening run is now taken atomically via a lookahead, which also fixes a correctness bug I wasn't looking for: CommonMark requires a closing fence be at least as long as its opener, so ````` is not closed by ```, but a backtracking opener shortened itself to three to force a pair and deleted the prose between them.

Both were caught by review, not by me, and neither would have been caught by reading the code.

The code-stripping logic is now three bounded replacements rather than the line scanner you'd have seen mid-review — 23 executable lines down to 8. Making only closing fences delimit blocks removed the need to encode CommonMark's rules for info strings, indent limits and closing markers, since each could only ever cause it to strip more, and unpaired markers already strip nothing.

Details worth a look

Prior art. nearform-actions/github-action-check-linked-issues (MIT) tackles the same GitHub limitation, which was useful confirmation the fallback is the right shape. @sugat009 read its source on the issue and found three reasons not to adopt it that are stronger than my own: its loose-matching is a mode switch rather than a fallback, so it ignores closingIssuesReferences entirely and a sidebar-linked PR reads as unlinked; it cannot do the assignee half of this check, so we would still fetch every issue ourselves on top of the GraphQL call; and its per-lookup bare catch turns a rate limit or 5xx into "not a valid issue", which is the bug this PR is fixing. Two things were worth taking from it: the optional colon in the keyword regex (Closes: #10 links on GitHub, so the fallback must accept it), and a no-issue skip label as a possible future escape hatch — out of scope here.

Ordering in the stripper is load-bearing. A  and delete the issue link between them — and an XML or HTML sample in a cht-core PR body makes that an ordinary body, not a contrived one. parseSections masks blocks for the same reason: that path failed the template check on the same input, which is a second bug the same root cause was causing.

Every removal leaves a newline behind. Deleting outright splices the prose either side together, so does not fix + #1234 reads as a reference. A space doesn't help, since the closing-reference regex spans those.

Case-insensitivity. GitHub owner and repo names are case-insensitive, so Closes Medic/cht-android#99 is a valid link. The org filter, the dedupe key and the same-repo comparison all compare case-insensitively, and the issue's canonical names are taken from the API response's repository_url rather than from what the contributor typed — otherwise a same-repo issue referenced as MEDIC/CHT-Core#1234 would render as MEDIC/CHT-Core#1234 instead of #1234 in the not-assigned message.

Failed lookups. Only 404 and 410 count as "the issue is not there". Anything else throws, so the job goes red with no comment and no label change — matching how the script already treats every other API call — and the next synchronize re-runs it. An earlier draft warned and skipped the check instead; @sugat009 pointed out on the issue that this was the one path that could hand a genuinely unlinked PR its Ready for review label.

Scope decisions, both confirmed on the issue: the fallback runs whenever closingIssuesReferences is empty rather than being gated on the base branch, so it behaves the same as the linkage path beside it. Matching is keyword-anchored only, so a "related to #123" aside does not register as a link.

Changed existing tests — worth reviewing that hunk closely. should fail when no issue is linked and should not count an issue linked from a repo outside the org both used a body containing Closes #1234. Under the new behaviour their names no longer describe what they test, so I repointed them at a new bodyWithoutIssue fixture. The stub for issues.get also matches owner/repo case-insensitively, because an exact-match double is stricter than the real API.

Known gaps, stated rather than left to be found

An unpaired fence strips nothing, so a body documenting Closes #N under a malformed or never-closed fence counts as a link. Deliberate, per the trade above, and still gated on the author being assigned to that issue.
Four-space-indented code blocks are not handled at all. A reference inside one counts. Same safe direction; never handled by any revision of this PR.
The fence indent bound (^ {0,3}) has no test. I wrote one and deleted it: the "only closing fences count" rule already subsumes that class, so the test only fired on a hand-built body with an indented opener and an unindented close. It pinned the regex, not a behaviour.
The ReDoS guard fails by hanging rather than failing. A synchronous regex blocks the event loop, so mocha's own timeout cannot fire; a regression there would stall the job until it times out rather than reporting a red test. The test still catches it, just noisily.

Tests

84 in andra-bot.spec.js. The full webapp mocha suite is 482 passing with 1 failure in tests/mocha/unit/testingtests/e2e/utils.spec.js:152 (expected Glob{…} to be an array) — a glob-library API shape, in a file this branch does not touch.

Tests assert what a contributor observes — whether the PR is failed and which label it gets — rather than whether a particular lookup happened. The exceptions are where the external call is the behaviour: that an out-of-org reference is never fetched, and that the 20-reference cap performs exactly 20 lookups.

Every new test was checked by mutating the source and confirming it goes red. Baseline 84 passing:

Mutation	Tests failed
Unpaired fence reaches end of body (the reverted design)	4
`REPO_NAME` ambiguously quantified (the cubic path)	job hangs — killed at 60s, against a 50ms baseline
`parseSections` stops masking code blocks	1
`stripNonProse` strips nothing	2
Opening fence run may backtrack shorter	1

This is worth doing rather than eyeballing: three tests I wrote during this round were vacuous on the first attempt and every one passed review by eye. Two put the issue link where it survived the mutation either way; one asserted a size limit using a reference that matched immediately and so never reached the expensive path at all. Your own mutation run on the previous revision is what taught me to do this, and it has since caught more than reading has.

Code review checklist

Readable: Concise, well named, follows the style guide
Tested: Unit and/or e2e where appropriate — see the Tests section above, including the mutation table showing each new test discriminates.
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes. — the fallback only runs when GitHub reports no linkage, so PRs that already pass are unaffected; no configuration or workflow changes.
AI disclosure: Please disclose use of AI per the guidelines.

AI Disclosure: Written with Claude Code (Claude Opus) — exploring the code path, drafting the fix and its tests, running the suites, and running the mutations above. I reviewed the result at each stage and I'm accountable for it.

That review is also where most of the defects above came from, and the pattern is worth disclosing plainly: several were introduced by a previous fix rather than being present in the original draft. The cubic REPO_NAME was written to close a review finding and was worse than the finding. The quadratic fence regex was written to close that one. Both looked correct, passed the suite, and were found only by measuring. Earlier revisions also had a capturing group in the keyword alternation that shifted every match index and produced issue_number: NaN — the lookup 404'd, the error was swallowed, and the check reported "not linked" exactly as before, so it looked like a working no-op.

I'd rather you review this knowing that than assume the current state arrived cleanly.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

The linked-issue check read GitHub's closingIssuesReferences, which is only populated for PRs targeting the default branch. On any other base the field is empty even when the contributor linked the issue correctly, so the check failed and told them to add a closing keyword they had already added. When GitHub reports no linkage, the PR body is now parsed for closing keywords and each reference is resolved through the issues API, so the existing same-org filter and assignee check keep working unchanged. - `medic#123`, `owner/repo#123` and full issue URLs are all recognised - comparisons are case-insensitive, since GitHub owner and repo names are - HTML comments are stripped first, so an unfilled template does not read as linked - a reference to a pull request is not a link - only a 404 means "no such issue"; any other lookup failure is reported as a warning rather than blaming the contributor for it

Per review on the issue: only 404 and 410 mean the referenced issue is not there. Everything else now throws, so the job goes red with no comment and no label change, matching how the script already treats every other API call, and the next synchronize re-runs it. The previous warn-and-skip behaviour was the one path that could hand a genuinely unlinked PR its "Ready for review" label.

Escaping every backslash twice made the pattern hard to read against GitHub's documented reference forms. The compiled regex is unchanged (verified byte-identical), so this is readability only. Fixes the SonarCloud javascript:S7780 findings.

sugat009

Reviewed this closely since it's the fix for a ticket I filed, and I ran the parsing claims rather than eyeballing them. The structure is good and the tests are thorough. Most of what follows is about the places where a hand-rolled parser has to match GitHub's linkifier exactly.

praise: 24 new cases covering every reference form, casing, dedupe, PR-vs-issue and the 404/410 split, plus the comment at scripts/ci/andra-bot.js:163 spelling out why other statuses are left to throw. That reasoning is right and it's rare to see it written down.

Two are worth settling before merge: the code-span gap at :116 and the early exit at :206. The rest are take-or-leave. It's a CI script and we can fix things when they bite, which is how we got here in the first place.

sugat009 · 2026-08-10T10:20:10Z

+// repository's default branch; on any other base the field is empty even though the
+// contributor linked the issue correctly. Parsing the body covers that case.
+const parseClosingReferences = (body, context) => {
+  const matches = stripComments(body || '').matchAll(CLOSING_REFERENCE_REGEX);


issue (blocking): code spans aren't excluded, so the gate can pass an unlinked PR

stripComments removes HTML comments but nothing else. I ran this regex over this PR's own body and it returns four references rather than one:

"Closes: #10" "Closes Medic/cht-android#99" "Closes #1234" "Closes #11324"

The first three are prose inside backticks. GitHub links none of them (checked against POST /markdown with mode: gfm).

Since getLinkedIssueFailure uses .some(), a PR with no real link passes as soon as its author is assigned to any org issue that happens to be mentioned in prose. That's the inverse of #11324: the bot green-lights something it should hold. Extending stripComments at :36 to drop fenced blocks and inline code spans covers it.

Side note: blockquotes are stripped, but GitHub does linkify inside them. Ignoring quoted text seems deliberate and it's defensible, it just isn't stated anywhere.

sugat009 · 2026-08-10T10:20:10Z

+  const isInOrg = owner => owner.toLowerCase() === context.repo.owner.toLowerCase();
+  const linkedIssues = result.repository.pullRequest.closingIssuesReferences.nodes
+    .filter(issue => isInOrg(issue.repository.owner.login));
+  if (linkedIssues.length) {


issue (blocking): the fallback exits one condition too early

This short-circuits on closingIssuesReferences being non-empty, but the gate is "linked and assigned to the author". A non-empty result isn't necessarily a passing one, so the body gets discarded in the case where it still matters.

The field has two independent feeds: closing keywords, which don't register on non-default branches, and the Development sidebar, which works on any branch. So on a non-default-branch PR a sidebar-linked epic makes this non-empty while the contributor's Closes #B is never read, and they get "You are not assigned to the linked issue (#epic)" with no edit that clears it. Same unfixable shape as #11324, relocated.

Being straight about evidence: this is a mechanism, not something I've seen happen.

Switching the condition costs nothing in the common case:

// today if (linkedIssues.length) { return linkedIssues; } // instead if (linkedIssues.some(isAssignedToAuthor)) { return linkedIssues; // happy path: body never parsed, no extra lookups }

The body is then read only when the bot is about to fail the PR anyway. Needs pr.user.login threaded in, since the assignment check currently lives in getLinkedIssueFailure.

sugat009 · 2026-08-10T10:20:11Z

+const CLOSING_KEYWORDS = 'close[sd]?|fix(?:e[sd])?|resolve[sd]?';
+const REPO_NAME = String.raw`[\w.-]+`;
+const CLOSING_REFERENCE_REGEX = new RegExp(
+  String.raw`\b(?:${CLOSING_KEYWORDS})\b\s*:?\s+` +


issue (non-blocking): \s*:?\s+ backtracks quadratically

Both quantifiers match whitespace, so the alternation is ambiguous. Measured against this exact regex, with closes followed by N spaces and a non-matching character:

N=2000 -> 4.2 ms N=20000 -> 460 ms N=65000 -> 4763 ms

GitHub's body limit is 65536, this runs under pull_request_target, and every edited event re-triggers it. Dropping the leading \s* gives 0.27 ms at N=65000 and still accepts both Closes #1 and Closes: #1 (verified both).

Same line, much smaller: \s+ matches newlines, so a keyword ending one line binds to a #N starting the next, which GitHub doesn't link. Zero occurrences in 4711 real cht-core PR bodies, so purely over-matching. [^\S\n]+ if you're touching the line anyway.

sugat009 · 2026-08-10T10:20:11Z

+  const references = [...matches].map(([, urlOwner, urlRepo, urlNumber, owner, repo, number]) => ({
+    owner: urlOwner || owner || context.repo.owner,
+    repo: urlRepo || repo || context.repo.repo,
+    number: Number(urlNumber || number),


issue (non-blocking): Number() on the raw digit run

Two verified behaviours: Number('01234') is 1234, so Closes #01234 resolves to #1234 even though GitHub doesn't autolink a leading-zero reference. And Number('9'.repeat(23)) stringifies to the literal '1e+23', which goes straight into the request path as /issues/1e+23.

What I did not verify is what GitHub returns for a non-integer path segment. If it's anything other than 404/410 the status isn't in MISSING_ISSUE_STATUSES, the error escapes getFailures, and the job goes red with no comment and no label change. Worth a Number.isSafeInteger guard regardless, since it's cheap.

sugat009 · 2026-08-10T10:20:11Z

+
+  const references = parseClosingReferences(context.payload.pull_request.body, context)
+    .filter(reference => isInOrg(reference.owner))
+    .slice(0, MAX_LINKED_ISSUES);


issue (non-blocking): truncation is silent and untested

References past the 20th are dropped with no log line. If the issue the author is assigned to sits 21st in document order they get "You are not assigned to the linked issue" listing 20 issues they aren't assigned to, and only reordering the body clears it.

Nothing exercises this either, so the .slice could be removed or moved before the org filter without a test failing. Also MAX_LINKED_ISSUES at :98 and the two hardcoded first: 20 values in the query are the same limit written three times.

sugat009 · 2026-08-10T10:20:11Z

+  }
+
+  const references = parseClosingReferences(context.payload.pull_request.body, context)
+    .filter(reference => isInOrg(reference.owner))


question (non-blocking): should the org gate run before resolution?

This filters on the owner string the contributor typed, whereas the GraphQL path at :204 filters after resolution. Since toIssueNode takes nameWithOwner from repository_url, an issue that has moved out of the org would come back under its new owner and never be re-filtered.

I couldn't test this: it needs a repo that has actually transferred out of medic. The staging difference between the two paths is real in the code, the exploitable consequence is my inference. Re-applying isInOrg to the resolved node would make the paths agree either way.

sugat009 · 2026-08-10T10:20:11Z

+      // A pull request is also an issue on this endpoint, but referencing one is not a link.
+      return data.pull_request ? null : toIssueNode(data, reference);
+    } catch (err) {
+      if (!MISSING_ISSUE_STATUSES.has(err.status)) {


question (non-blocking): does 404 always mean "doesn't exist"?

GitHub also returns 404 when the token can't see the repository, and GITHUB_TOKEN only has public-level access outside this repo. So Closes medic/<private-repo>#42 would be dropped with a core.info line and the PR reported unlinked, which is the false-fail class this PR exists to remove, with the reason visible only in the job log.

I didn't test this, since it needs a private in-org repo. If it holds, surfacing dropped references in the comment rather than only the log would make it diagnosable.

sugat009 · 2026-08-10T10:20:11Z

+// `#123`, `owner/repo#123`, and a full issue URL.
+// https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue
+const CLOSING_KEYWORDS = 'close[sd]?|fix(?:e[sd])?|resolve[sd]?';
+const REPO_NAME = String.raw`[\w.-]+`;


question (if-minor): should REPO_NAME accept . and ..?

[\w.-]+ matches .., so Closes medic/..#1 parses as owner=medic repo=.. num=1 (verified) and passes isInOrg, since the owner really is medic. It then reaches issues.get with repo: '..'.

I haven't checked what the client does with that path, so I'm not claiming an impact beyond a stray 404. It's contributor-controlled text going into an API path in a privileged workflow, so requiring at least one non-dot character seems worth the two characters it costs.

sugat009 · 2026-08-10T10:20:11Z

+      expect(commentBody).to.contain(getMessage('missing-linked-issue'));
+    });
+
+    it('should ignore a reference to an issue that does not exist', async () => {


issue (non-blocking): this test and the 410 one pass vacuously

Both rely on the beforeEach stub rejecting and assert only that the comment contains missing-linked-issue, never that a lookup was attempted.

I checked by mutation: at PR head the suite is 67 passing; making parseClosingReferences return [] so body parsing does nothing turns 14 tests red, but these two stay green:

✔ should ignore a reference to an issue that does not exist ✔ should ignore a reference to an issue that was deleted or transferred

That's the same class as the issue_number: NaN regression the description says an earlier draft hit. expect(github.rest.issues.get.called).to.be.true; fixes both, the way the outside-the-org test at :436 already does in the negative direction.

…links Review findings on medic#11332. Two that could pass or fail a PR wrongly: - Code spans and fenced blocks were parsed. Because the assignment check uses `.some()`, a PR with no real link passed as soon as its author was assigned to any org issue merely mentioned in prose. Run over this PR's own body the parser found four references where GitHub links one. - The fallback exited on `closingIssuesReferences` being non-empty, but the gate is "linked and assigned". That field is also fed by the Development sidebar, which works on any base branch, so a sidebar-linked epic could hide a contributor's own keyword link behind a failure they could not clear. It now exits only on a link that would actually pass, and the two sources are merged rather than one replacing the other. Smaller ones from the same review: - `\s*:?\s+` was ambiguous and backtracked quadratically, ~4s at GitHub's body limit in a workflow re-run on every edit. `:?[^\S\n]+` is 0.37ms and no longer binds a keyword across a newline. - Issue numbers with leading zeros or beyond safe-integer range are rejected rather than reaching the API as `1234` or `1e+23`. - A repo name of only dots can no longer reach the API path. - References past the limit are logged instead of silently dropped, and the limit is defined once rather than three times. - Resolved issues are re-checked against the org, matching the GraphQL path, so an issue transferred out cannot slip through. - A dropped reference warns rather than infos: 404 also means the token cannot see the repository.

ken-talltree-io · 2026-08-10T16:01:21Z

Both blocking findings fixed, and I took all seven non-blocking ones. Reproduced each before changing anything — your numbers hold.

Code spans (:116). You're right, and I want to be straight about this one: I flagged the gap in the PR description and dismissed it as "largely self-limiting, since the issue must exist and be assigned to the PR author." That reasoning was wrong because I missed the .some() interaction you point out — being assigned to any mentioned issue is enough, so nothing limits it. Extended the stripping to fenced blocks and inline spans, fences first so a backtick inside one can't pair with a later one. Run against this PR's own body it now finds 1 reference where it found 4.

Blockquotes: agreed that ignoring quoted text is defensible but unstated. Since GitHub does linkify there, I've left the behaviour alone rather than change it in passing — worth its own decision.

Early exit (:206). Switched to linkedIssues.some(isAssignedTo(…, author)), with pr.user.login threaded through. The body is now read only when the bot is about to fail anyway, so the happy path costs nothing extra. One thing beyond what you suggested: rather than the parsed set replacing the GraphQL set, they're merged — otherwise a sidebar-linked epic would vanish from the not-assigned list and the contributor would lose the information about what it's actually linked to. That needed dedupe in getLinkedIssueFailure, since both sources can name the same issue.

Backtracking (:106). Reproduced at 4175ms for N=65000 on my machine, 0.37ms after. :?[^\S\n]+ — accepts Closes #1 and Closes: #1, and no longer binds across a newline.

Number() (:120). Both behaviours confirmed. Now requires /^[1-9]\d*$/ and Number.isSafeInteger, so leading zeros and oversized runs are dropped before any request. I didn't chase what GitHub returns for a non-integer path segment — the guard makes it moot.

Truncation (:212). Now warns with the count dropped, and MAX_LINKED_ISSUES is passed into the query as a variable rather than being written three times.

Org gate (:211). Resolved nodes are re-filtered with the same isInOrg, so both paths now filter after resolution. I couldn't construct a transferred-out-of-org repo either; the change makes the paths agree regardless.

404 (:166). Can't test the private-repo case without a private in-org repo, so I haven't claimed it's fixed — but the reasoning holds, so dropped references now core.warning rather than core.info and say "not found or not visible". Surfacing them in the PR comment is a bigger UX change and I'd rather not fold it in here.

REPO_NAME (:104). [\w.-]*[\w-][\w.-]* — at least one non-dot character.

The vacuous tests. Confirmed by your mutation and fixed with the issues.get.called assertion. Then I ran the same technique across the rest of the fixes, which I should have been doing from the start — baseline 474 passing / 1 pre-existing failure:

mutation	result
`parseClosingReferences` returns `[]`	19 failing
`stripCode` removed	3 failing
early exit back to `.length`	2 failing
regex back to `\s*:?\s+`	2 failing
number guard removed	2 failing

Every fix is pinned by at least one test that fails without it. 9 new cases; lint clean.

The regex version I pushed had three bugs, two of which reintroduced the failure classes it was added to prevent: - One unbalanced backtick paired with the next one anywhere below it, including the PR template's own, deleting a real `Closes medic#1234` in between and failing a correctly linked PR. - A fence with no matching close, or a `~~~` block closed with backticks, was parsed as prose. GitHub renders both as code and links nothing, so an unlinked PR could pass. - Removing a region spliced the prose either side together, so "does not fix `anything` medic#1234" read as a reference. Every rule here is a line-level rule, so a line scanner states each one directly instead of encoding it in a backreference. Removals leave a newline, which the closing-reference regex will not cross — that also makes a single pass over comments sufficient. Fences do not nest, so one open marker is tracked rather than a stack.

Review found four ways the line scanner discarded a genuine issue link, all of them false negatives that fail a contributor with no way to clear the failure. Three shared a root cause: an unmatched fence reached to the end of the body, so one misread line hid every reference below it. Only a fence that actually closes now delimits a block, which leaves an unpaired marker stripping nothing and replaces the scanner with three bounded replacements. That removes the need to encode CommonMark's rules for info strings, indented fences and closing markers, since each could only ever cause this to strip more. Order matters: a `` and delete the link between them. parseSections masks blocks for the same reason. Restore the linear repo-name pattern. Requiring a non-dot character made its quantifiers ambiguous and the failed match cubic: 68s on a 8k body and hours at GitHub's 65536-character limit, on a pull_request_target body re-parsed on every edit. `..` is rejected after parsing instead. Two tests asserted the old unbounded behaviour and now assert the new contract. Assertions moved from `issues.get.called` to the outcome the contributor sees. A test pinning the fence indent bound was dropped as vacuous: the pair requirement already subsumes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The opening run could be retried shorter, which was wrong before it was slow. CommonMark requires a closing fence be at least as long as its opener, so ````` is not closed by ```; backtracking the opener down to three found a pair anyway and deleted the prose between them, losing a real issue link. Each candidate length also rescans the line, so the failed match was quadratic: 3.5s on a body of backticks at GitHub's 65536-character limit, against 0.1ms once the run is taken whole. The previous line scanner was linear here, so this was a regression introduced with it. A lookahead makes the run atomic. Behaviour is unchanged on balanced blocks, unpaired markers, tilde fences, indented markers and longer closes; the only difference is the mis-pairing above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

sugat009

Re-review at f7702cd1. Reproduced every fix before signing off; all nine hold.

praise: you didn't just take the findings, you ran the mutation technique across all of them and pinned each fix with a test that fails without it. That's more rigor than the review asked for.

The one worth calling out. I suggested splitting REPO_NAME into [\w.-]*[\w-][\w.-]* to require a non-dot character. You rejected that and kept [\w.-]+ with a post-parse /^\.+$/ filter instead, documenting why. You were right, and it's worse than I realized: my suggested pattern makes the two quantifiers able to divide a word run, so with two names either side of the / it scales cubically. Measured, with closes + a×N + / + a×N + ! (no trailing #N, to force a full backtrack):

N=200   ->     93 ms
N=1200  ->   1988 ms
N=2400  ->  16147 ms

Your version stays at ~0.03 ms throughout. So the "simple" fix would have reintroduced a worse ReDoS than the one it closed. Good catch, and the comment explaining it is exactly the kind that saves the next person.

Verified fixes:

finding	check
code spans (`:116`)	`stripNonProse` over this PR's own body now yields 1 reference (`Closes #11324`), was 4
early exit (`:206`)	short-circuits only on an assigned link; merges rather than replaces, so the `not-assigned` list keeps a sidebar-linked issue
backtracking (`:106`)	`:?[^\S\n]+` is 0.27 ms at 65k, still accepts `Closes #1` and `Closes: #1`, no longer binds across a newline
`Number()` (`:120`)	`01234` and the 23-digit run both drop to `NaN` before any request
org gate (`:211`)	resolved nodes re-filtered with `isInOrg`, so both paths filter after resolution
truncation (`:212`)	warns with the dropped count; `MAX_LINKED_ISSUES` is now the query's `$limit`
404 (`:166`)	`core.warning` with "not found or not visible"
`REPO_NAME` (`:104`)	above
vacuous tests	`issues.get.called` assertions added; confirmed by re-running the `parseClosingReferences -> []` mutation

On the stripper rewrite. I probed the line scanner for the failure that would matter, a real prose reference wrongly stripped as code, and couldn't produce one. The lazy [\s\S]*? pairs a fence opener with the first close, so prose between two blocks survives; an unclosed fence strips nothing; unbalanced backticks don't consume a trailing reference. Everything ambiguous resolves toward stripping less, which is the safe direction. The blockquote question stays open as its own decision; agreed it shouldn't ride along here.

Approving. Nice work.

ken-talltree-io added 2 commits August 6, 2026 08:19

github-actions Bot added the Ready for review label Aug 7, 2026

sugat009 reviewed Aug 10, 2026

View reviewed changes

ken-talltree-io and others added 3 commits August 10, 2026 09:22

sugat009 approved these changes Aug 11, 2026

View reviewed changes

sugat009 assigned ken-talltree-io Aug 11, 2026

Navigation Menu

Uh oh!

Uh oh!

fix(#11324): recognise closing keywords on non-default-branch PRs - #11332

fix(#11324): recognise closing keywords on non-default-branch PRs#11332
ken-talltree-io wants to merge 7 commits into
medic:masterfrom
ken-talltree-io:11324-andra-bot-linked-issue-fallback

ken-talltree-io commented Aug 7, 2026 •

edited

Loading

Uh oh!

sugat009 left a comment

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

sugat009 Aug 10, 2026

Uh oh!

ken-talltree-io commented Aug 10, 2026

Uh oh!

sugat009 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

ken-talltree-io commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Since your review, @sugat009

Details worth a look

Known gaps, stated rather than left to be found

Tests

Code review checklist

License

Uh oh!

sugat009 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken-talltree-io commented Aug 10, 2026

Uh oh!

sugat009 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ken-talltree-io commented Aug 7, 2026 •

edited

Loading